Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for S2.FX.Parallel to notify aggregated objects about steps. #27

Merged
merged 1 commit into from Apr 18, 2011
Merged

Fix for S2.FX.Parallel to notify aggregated objects about steps. #27

merged 1 commit into from Apr 18, 2011

Conversation

rafalwrzeszcz
Copy link
Contributor

There is a problem with S2.FX.Parallel that it doesn't invoke methods on aggregated effects like start() or cancel(), so it's callbacks won't be executed. What's more, effects can even not be completed, since cancel() is not called, which causes teardown() to not be called.

I believe those are two methods that have to be called on sub-effects to make them fully functional (start() marks effects as "running" as cancel() won't take any effect if status is not "running") - i don't see any need to call finish() on sub-effects (but if it would be needed, then canel() should not be called - only one of them should be called by parallel aggregator).

madrobby added a commit that referenced this pull request Apr 18, 2011
---

There is a problem with S2.FX.Parallel that it doesnt invoke methods on aggregated effects like start() or cancel(), so its callbacks wont be executed. Whats more, effects can even not be completed, since cancel() is not called, which causes teardown() to not be called.

I believe those are two methods that have to be called on sub-effects to make them fully functional (start() marks effects as "running" as cancel() wont take any effect if status is not "running") - i dont see any need to call finish() on sub-effects (but if it would be needed, then canel() should not be called - only one of them should be called by parallel aggregator).
@madrobby madrobby merged commit 2c0b34b into madrobby:master Apr 18, 2011
@madrobby
Copy link
Owner

I've applied this—please think about adding tests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants